-
Notifications
You must be signed in to change notification settings - Fork 8.3k
soc: silabs: siwx91x: Improve performance #98060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
soc: silabs: siwx91x: Improve performance #98060
Conversation
7487afc to
6cf1b1d
Compare
drivers/wifi/siwx91x/Kconfig.siwx91x
Outdated
| help | ||
| Enable this option to improve the throughput performance of the SiWx91x | ||
| series. This feature provides enhanced data transfer capabilities and | ||
| improved network performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why anyone wouldn't want to enable this option? If there is not reason to not enable it, maybe we don't need a config parameter.
(BTW, this symbol is orphan. You can definitely remove it then).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is just a serie a paraphrase of the title. You have to guide the user in his choice: why he should want to not enable it.
Typically, it would probably makes sense to warn will break support for WPA3 and MPF (so so generate some security weakness).
It would be nice to include a couple of number if you have ones.
| SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID) | ||
| : (SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID | | ||
| SL_SI91X_CUSTOM_FEAT_ASYNC_CONNECTION_STATUS | | ||
| SL_SI91X_CUSTOM_FEAT_RTC_FROM_HOST), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need the condition? Why not just
.custom_feature_bit_map =
SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID |
SL_SI91X_CUSTOM_FEAT_SOC_CLK_CONFIG_160MHZ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is still relevant.
92977e7 to
576dc52
Compare
drivers/wifi/siwx91x/Kconfig.siwx91x
Outdated
| help | ||
| Enable this option to improve the throughput performance of the SiWx91x | ||
| series. This feature provides enhanced data transfer capabilities and | ||
| improved network performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is just a serie a paraphrase of the title. You have to guide the user in his choice: why he should want to not enable it.
Typically, it would probably makes sense to warn will break support for WPA3 and MPF (so so generate some security weakness).
It would be nice to include a couple of number if you have ones.
| SL_SI91X_EXT_FEAT_FRONT_END_SWITCH_PINS_ULP_GPIO_4_5_0; | ||
| if (!IS_ENABLED(CONFIG_WIFI_SIWX91X_HIGH_THROUGHPUT)) { | ||
| boot_config->ext_custom_feature_bit_map |= SL_SI91X_EXT_FEAT_IEEE_80211W; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you confirm the rest of the driver still behave properly in the various MFP scenario (I assume sl_wifi_set_mfp() will return an error and everything is fine).
BTW, I think disabling 11w should be a separated parameters (I don't think many user want it). Do you know what is the performance benefit of this parameter alone?
| SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID) | ||
| : (SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID | | ||
| SL_SI91X_CUSTOM_FEAT_ASYNC_CONNECTION_STATUS | | ||
| SL_SI91X_CUSTOM_FEAT_RTC_FROM_HOST), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is still relevant.
42c44e2 to
f226c12
Compare
| } | ||
| .ta_pool = {.tx_ratio_in_buffer_pool = 1, | ||
| .rx_ratio_in_buffer_pool = 1, | ||
| .global_ratio_in_buffer_pool = 1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New line after opening brace:
.ta_pool = {
.tx_ratio_in_buffer_pool = 1,
.rx_ratio_in_buffer_pool = 1,
.global_ratio_in_buffer_pool = 1,
}
I have not been to understand how these ratio work despite the documentation in sl_wifi_device.h. If all the values are identical, every buffer type has one third of the memory? Then, how the new values are different from the previous ones?
Or every buffer type get x/10 of the memory? Then, how it worked when these values were 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record: when this structure was filled with zeros, the NWP only reserved a small fixed numbers (~10) of RX/TX buffers. With the new code, the number of buffers is a ratio (1/3 Tx, 1/3 Rx) of the available RAM.
f226c12 to
db5eaa1
Compare
db5eaa1 to
937f705
Compare
| * to NWP, better are the WiFi and BLE performances. | ||
| */ | ||
| reg = <0x00000400 DT_SIZE_K(255)>; | ||
| reg = <0x00000400 DT_SIZE_K(319)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand right, you give less memory to NWP and you got better perf ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original values has been computed to offload socket/TCP/IP to the NWP. However, since the Zephyr is taking care about these layers, the NWP is less solicited than initially expected. So, it makes sense to allocate more memory to the M4. Probably the user would want to allocate more memory to the NWP if he enables CONFIG_WIFI_SILABS_SIWX91X_NET_STACK_OFFLOAD.
@ragurram26, maye the sentence "Less memory is allocated to Zephyr, more memory is allocated to NWP, better are the WiFi and BLE performances." is not accurate anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ragurram26 don't you believe the comment should be updated?
| * to NWP, better are the WiFi and BLE performances. | ||
| */ | ||
| reg = <0x00000400 DT_SIZE_K(255)>; | ||
| reg = <0x00000400 DT_SIZE_K(319)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original values has been computed to offload socket/TCP/IP to the NWP. However, since the Zephyr is taking care about these layers, the NWP is less solicited than initially expected. So, it makes sense to allocate more memory to the M4. Probably the user would want to allocate more memory to the NWP if he enables CONFIG_WIFI_SILABS_SIWX91X_NET_STACK_OFFLOAD.
@ragurram26, maye the sentence "Less memory is allocated to Zephyr, more memory is allocated to NWP, better are the WiFi and BLE performances." is not accurate anymore?
3577df3 to
93eeeee
Compare
| - deep-sleep-with-ram-retention | ||
| required: true | ||
|
|
||
| soc-120mhz-clk: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a standard naming for this property? If it's specific to Silabs, perhaps it should have some appropriate prefix? (e.g. silabs,)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like a clock-frequency property would be better, potentially with an enum to limit the selection to allowed values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, I am a bit confused by the name. What soc means here? Is it clock the NWP clock? If this parameter is not set what is the frequency of the NWP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SL_SI91X_CUSTOM_FEAT_SOC_CLK_CONFIG_160MHZ also exists. Why we don't use 160MHz?
I assume 160MHZ is exclusive wit 120MHZ, so a boolean is probably not the right type.
Dismissing my review until my question is answered
93eeeee to
ea8b5f6
Compare
To enhance throughput performance on the SiWx91x series, Added some Siwx91x configurations by deafult. Signed-off-by: Rahul Gurram <[email protected]>
Added support for 120Mhz soc clock via DTS.Setting the clock to 120 MHz can improve throughput Signed-off-by: Rahul Gurram <[email protected]>
ea8b5f6 to
26af633
Compare
|
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
To enhance throughput performance on the SiWx91x series, certain configurations need to be enabled in the SiWx91x.